Skip to content

test: set test-process-finalization as flaky#63057

Open
IlyasShabi wants to merge 1 commit intonodejs:mainfrom
IlyasShabi:ishabi/issue-63056
Open

test: set test-process-finalization as flaky#63057
IlyasShabi wants to merge 1 commit intonodejs:mainfrom
IlyasShabi:ishabi/issue-63056

Conversation

@IlyasShabi
Copy link
Copy Markdown
Member

Set test-process-finalization as flaky

#63056

@IlyasShabi IlyasShabi added flaky-test Issues and PRs related to the tests with unstable failures on the CI. windows Issues and PRs related to the Windows platform. labels Apr 30, 2026
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 30, 2026
Signed-off-by: ishabi <ilyasshabi94@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.63%. Comparing base (80e0f14) to head (3bcacb1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63057      +/-   ##
==========================================
- Coverage   89.65%   89.63%   -0.02%     
==========================================
  Files         708      708              
  Lines      220413   220413              
  Branches    42275    42280       +5     
==========================================
- Hits       197607   197571      -36     
- Misses      14658    14704      +46     
+ Partials     8148     8138      -10     

see 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@inoway46
Copy link
Copy Markdown
Contributor

inoway46 commented May 3, 2026

@IlyasShabi
Thanks for working on this. I think this can be fixed in the fixture instead of marking the test as flaky.

different-registry-per-thread.mjs registers object literals with process.finalization.register(), but those objects are not strongly referenced after registration. process.finalization only keeps a WeakRef, and the documented behavior is that the callback is not called on process exit if the ref was GCed before exit:

node/doc/api/process.md

Lines 2108 to 2111 in b2f6aa3

This function registers a callback to be called when the process emits the `exit`
event if the `ref` object was not garbage collected. If the object `ref` was garbage collected
before the `exit` event is emitted, the callback will be removed from the finalization registry,
and it will not be called on process exit.

That matches the failure here: the child process exits with status 0, but stdout only contains shutdown on worker.

Keeping the registered refs alive until exit should make this deterministic, e.g. by storing the main-thread and worker refs in a module-level array before registering them.

The V8 update may still be related, but it seems more likely that it changed GC timing enough to expose this fragile test assumption.

@inoway46
Copy link
Copy Markdown
Contributor

inoway46 commented May 3, 2026

One concrete shape for the fixture fix could be to keep the registered refs alive in a top-level array, for both the main thread and the worker.

const registeredRefs = [];

const ref = { foo: 'foo' };
registeredRefs.push(ref);
process.finalization.register(ref, () => {
  process.stdout.write('shutdown on main thread\n');
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test Issues and PRs related to the tests with unstable failures on the CI. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants